Skip to content

[OTLP] Limit response body size read by exporters#7017

Merged
martincostello merged 9 commits intoopen-telemetry:mainfrom
martincostello:limit-response-size-read
Apr 6, 2026
Merged

[OTLP] Limit response body size read by exporters#7017
martincostello merged 9 commits intoopen-telemetry:mainfrom
martincostello:limit-response-size-read

Conversation

@martincostello
Copy link
Copy Markdown
Member

@martincostello martincostello commented Mar 30, 2026

See open-telemetry/opentelemetry-proto#781.

Changes

Limit the length of the HTTP response body that is read if export fails for gRPC or HTTP.

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

@martincostello
[OTLP] Limit response body size read by exporters
18b5811 
Limit the length of the HTTP response body that is read if export fails for gRPC or HTTP.

See open-telemetry/opentelemetry-proto#781.
martincostello
martincostello commented Mar 30, 2026
View reviewed changes
@martincostello
[OTLP] Update CHANGELOG
ba293a4 
Add PR number.
@github-actions github-actions bot added the pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package label Mar 30, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 30, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 85.71429% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.78%. Comparing base (0f74489) to head (31336b7).
⚠️ Report is 15 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...ol/Implementation/ExportClient/OtlpExportClient.cs 93.54% 2 Missing ⚠️
...mplementation/ExportClient/OtlpGrpcExportClient.cs 75.00% 2 Missing ⚠️
...mplementation/ExportClient/OtlpHttpExportClient.cs 33.33% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7017      +/-   ##
==========================================
- Coverage   88.87%   88.78%   -0.09%     
==========================================
  Files         263      263              
  Lines       12424    12451      +27     
==========================================
+ Hits        11042    11055      +13     
- Misses       1382     1396      +14
Flag Coverage Δ
unittests-Project-Experimental 88.54% <85.71%> (-0.22%) ⬇️
unittests-Project-Stable 88.39% <85.71%> (-0.45%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ol/Implementation/ExportClient/OtlpExportClient.cs 97.18% <93.54%> (-2.82%) ⬇️
...mplementation/ExportClient/OtlpGrpcExportClient.cs 74.46% <75.00%> (-1.88%) ⬇️
...mplementation/ExportClient/OtlpHttpExportClient.cs 90.00% <33.33%> (-10.00%) ⬇️

... and 2 files with indirect coverage changes

@martincostello martincostello marked this pull request as ready for review March 30, 2026 14:12
@martincostello martincostello requested a review from a team as a code owner March 30, 2026 14:12
Copilot AI review requested due to automatic review settings March 30, 2026 14:12
Copilot AI reviewed Mar 30, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Limits the amount of HTTP response body read (for error logging) when OTLP export fails, aligning exporter behavior with updated OTLP guidance.

Changes:

  • Add size-capped response-body reading helper (TryGetResponseBody) and unit tests for truncation/edge cases.
  • Avoid reading the response body unless EventSource error logging is enabled (HTTP and gRPC exporters).
  • Update exporter changelog to document the behavioral change.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpExportClientTests.cs Adds tests validating TryGetResponseBody behavior, including truncation and exception handling.
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/ExportClient/OtlpHttpExportClient.cs Gates response-body reading on EventSource being enabled to avoid unnecessary work.
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/ExportClient/OtlpGrpcExportClient.cs Same gating for gRPC; minor refactor to expression-bodied helper.
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/ExportClient/OtlpExportClient.cs Implements capped response-body reading and refactors SendHttpRequest to expression-bodied form.
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/CHANGELOG.md Documents the new response-body read limiting behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@martincostello
[OTLP] Fix method name
69909e8 
Fix test method name.
@pellared pellared mentioned this pull request Mar 30, 2026
Merged
4 tasks
@martincostello
[OTLP] Address review feedback
c92243b 
- Eagerly read until no more data or limit reached.
- Read bytes and convert to string respecting encoding.
- Avoid reading content if `CancellationToken` is signalled.
@martincostello
[OTLP] Update default limit
f8070b6 
Update limit from 32KiB to 4MiB.
@martincostello
[OTLP] Rent buffer
c7bf057 
When targeting .NET, rent a buffer rather than allocate.
@martincostello
[OTLP] Add test for compression
0d7bd6c 
Add a test that verifies the behaviour if the content is compressed.
@martincostello
[OTLP] Address feedback
bb18cb2 
Move `ArrayPool<T>.Return(...)` call into `finally`.
@martincostello
[OTLP] Add truncation suffix
31336b7 
Add a suffix of `[TRUNCATED]` if the response body is truncated.
@martincostello
Copy link
Copy Markdown
Member Author

martincostello commented Mar 31, 2026

I'll merge this once open-telemetry/opentelemetry-proto#781 is merged, just in case there's any last-minute changes.

@martincostello
Copy link
Copy Markdown
Member Author

martincostello commented Apr 6, 2026

The documentation change hasn't been merged yet, but both Go and Kotlin have merged the equivalent PRs, so I think we're good to merge this.

@martincostello martincostello added this pull request to the merge queue Apr 6, 2026
Merged via the queue into open-telemetry:main with commit 63f220b Apr 6, 2026
54 checks passed
@martincostello martincostello deleted the limit-response-size-read branch April 6, 2026 11:19
This was referenced Apr 8, 2026
Open
Closed
This was referenced Apr 13, 2026
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Merged
Open
Open
Open
Merged
Merged
Merged
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@Kielek Kielek Kielek approved these changes

@rajkumar-rangaraj rajkumar-rangaraj rajkumar-rangaraj approved these changes

Assignees

No one assigned

Labels

pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@martincostello @Kielek @rajkumar-rangaraj